fix(ambient): remove allowed-tools restriction so skills actually load by dean0x · Pull Request #152 · dean0x/devflow

dean0x · 2026-03-20T08:24:08Z

Summary

Root cause: ambient-router skill had allowed-tools: Read, Grep, Glob which prevented Claude from invoking the Skill tool — skills were classified but never loaded
Removed allowed-tools from ambient-router (unrestricted access as main-session orchestrator)
Added override note in Step 4 <IMPORTANT> block so loaded skills' allowed-tools don't restrict main session capabilities
Strengthened hook preamble with explicit Skill tool mention for GUIDED/ORCHESTRATED
Added hasSkillLoading() and extractLoadedSkills() integration test helpers + 2 verification tests

Test plan

npm run build — 35 skills distributed across 17 plugins
npm test — 250/250 pass
npm run test:integration — verify GUIDED/ORCHESTRATED prompts show "Loading:" marker (requires claude CLI)
Manual: new session → GUIDED prompt → confirm Skill tool invocations in output

The ambient-router skill had `allowed-tools: Read, Grep, Glob` which prevented Claude from invoking the Skill tool to load skills for GUIDED/ORCHESTRATED classifications. Removes the restriction (router needs unrestricted access as the main-session orchestrator), adds an override note so loaded skills' allowed-tools don't restrict the main session, strengthens the hook preamble, and adds integration test helpers for skill loading verification.

The integration tests (claude -p) can't reliably trigger ambient classification since UserPromptSubmit hooks don't fire in non-interactive mode. Added 11 unit tests for the regex helpers (hasClassification, extractIntent, extractDepth, hasSkillLoading, extractLoadedSkills) that verify the parsing logic without API calls. Updated integration test docs to explain the -p mode limitation.

Adds --dry-run option that shows what would be removed (skills, agents, commands, extras) without performing any destructive operations. Works for both full and selective uninstalls. Includes formatDryRunPlan pure function with deduplication and 4 unit tests.

dean0x · 2026-03-20T10:16:38Z

tests/integration/ambient-activation.test.ts

  });
+
+  // Skill loading verification — GUIDED should show "Loading:" marker
+  it('loads skills for GUIDED classification', () => {


Issue: Flaky Integration Tests (85% confidence)

This test asserts on non-deterministic LLM classification in -p mode. The file acknowledges classification may not appear, yet the test uses hard expect(...).toBe(true) without conditional fallback.

Fix: Use it.skip() or conditional assertions:

if (hasClassification(output)) { expect(hasSkillLoading(output)).toBe(true); }

Claude Code Review

dean0x · 2026-03-20T10:16:40Z

tests/integration/ambient-activation.test.ts

+  });
+
+  // Skill loading verification — ORCHESTRATED should show "Loading:" marker
+  it('loads skills for ORCHESTRATED classification', () => {


Issue: Flaky Integration Tests (85% confidence)

Same as GUIDED test above — this assertion may fail in -p mode due to unreliable classification.

Fix: Skip or use conditional assertions (see line 73 comment).

Claude Code Review

dean0x · 2026-03-20T10:16:42Z

tests/integration/helpers.ts

  }
 }

+const AMBIENT_PREAMBLE =


Issue: Duplicated Magic String (82% confidence)

The AMBIENT_PREAMBLE is defined identically here and in scripts/hooks/ambient-prompt:42. These can drift if one is updated without the other.

Fix: Add a test that verifies the preamble substring exists in the hook script:

it('preamble matches ambient-prompt hook script', () => { const hookScript = readFileSync( path.join(__dirname, '../../scripts/hooks/ambient-prompt'), 'utf-8', ); expect(hookScript).toContain(AMBIENT_PREAMBLE); });

Claude Code Review

dean0x · 2026-03-20T10:16:55Z

src/cli/commands/uninstall.ts

+      if (!isSelectiveUninstall) {
+        const docsDir = path.join(process.cwd(), '.docs');
+        const memoryDir = path.join(process.cwd(), '.memory');
+        try { await fs.access(docsDir); extras.push('.docs/'); } catch { /* noop */ }


Issue: Sequential I/O Performance (82% confidence)

The two fs.access checks for .docs/ and .memory/ are awaited sequentially. These are independent filesystem operations that could run in parallel.

Fix: Use Promise.allSettled:

const [docsResult, memoryResult] = await Promise.allSettled([ fs.access(docsDir), fs.access(memoryDir), ]); if (docsResult.status === 'fulfilled') extras.push('.docs/'); if (memoryResult.status === 'fulfilled') extras.push('.memory/');

Claude Code Review

dean0x · 2026-03-20T10:16:58Z

tests/uninstall-logic.test.ts

  });
 });
+
+describe('formatDryRunPlan', () => {


Issue: Untested Deduplication Logic (82% confidence)

The formatDryRunPlan function explicitly deduplicates inputs with [...new Set(assets.skills)], but no test verifies this behavior. If the dedup were removed, no test would catch it.

Fix: Add test case:

it('deduplicates repeated asset names', () => { const plan = formatDryRunPlan({ skills: ['core-patterns', 'core-patterns', 'typescript'], agents: ['coder', 'coder'], commands: ['/implement'], }); expect(plan).toContain('Skills (2)'); expect(plan).toContain('Agents (1)'); });

Claude Code Review

dean0x · 2026-03-20T10:17:33Z

Code Review Summary

Branch: fix/ambient-skill-loading → main
Commits: 3 | Changed Files: 9
Review Date: 2026-03-20

Inline Comments Created (6)

Flaky Integration Tests (lines 73, 82) — GUIDED/ORCHESTRATED tests assert on unreliable -p mode classification. Use it.skip() or conditional assertions.
Duplicated Preamble (line 18, helpers.ts) — String duplicated with ambient-prompt hook. Add test to verify sync.
Sequential I/O (line 206, uninstall.ts) — Two fs.access checks should use Promise.allSettled for parallelism.
Missing Test (line 68, uninstall-logic.test.ts) — formatDryRunPlan deduplication untested.

Additional Findings (60-79% confidence, see details below)

Documentation Issues:

README missing --dry-run, --plugin, --verbose flags in Uninstall Options table
CHANGELOG [Unreleased] empty — needs entries for new features/fixes
CLAUDE.md wording could clarify that ambient-router omits allowed-tools entirely

Architecture/Design:

uninstall.ts is a 610-line monolith (pre-existing, not introduced here)
formatDryRunPlan could extract to utility module but is well-tested
Unit test imports from tests/integration/helpers.js (boundary violation, pre-existing import)

Test Coverage:

Missing tests for extractIntent with EXPLORE and CHAT classifications
Integration test comment is verbose but well-documented

Performance/Robustness:

Pre-existing: Shell hook invokes jq three times per prompt (optimization opportunity)
Pre-existing: execSync uses string interpolation (should use execFileSync)
Suggestion: Dedup in computeAssetsToRemove rather than formatDryRunPlan

Security (Defense-in-Depth):

ambient-router has no allowed-tools restriction (intentional, documented, but defense-in-depth concern)

Recommendation

APPROVED WITH CONDITIONS

Merge after addressing:

HIGH PRIORITY: Fix flaky integration tests (skip or condition assertions)
MEDIUM PRIORITY: Verify preamble sync or add test
OPTIONAL: Add CHANGELOG entries, update README docs

All three commits are atomic and well-tested. No blocking regressions. The ambient skill-loading fix is correct and necessary.

Detailed Review Reports: .docs/reviews/fix-ambient-skill-loading/

- Skip flaky GUIDED/ORCHESTRATED integration tests (non-deterministic in -p mode) - Add security comment to ambient-router SKILL.md (no allowed-tools is deliberate) - Add --dry-run, --plugin, --verbose to README uninstall options table - Add CHANGELOG entries for --dry-run flag and ambient skill loading fix - Add SYNC cross-reference comments between ambient-prompt and helpers.ts - Add preamble drift-detection test (shell script vs helpers.ts) - Clarify dry-run scope log with "(dry-run shows all detected scopes)" - Add formatDryRunPlan deduplication test - Add EXPLORE/CHAT intent coverage to extractIntent tests - Replace echo with printf in ambient-prompt for POSIX correctness - Replace execSync with execFileSync to avoid shell injection - Fix CLAUDE.md wording: "has no" → "omits ... entirely"

Dean Sharon added 3 commits March 20, 2026 10:23

dean0x commented Mar 20, 2026

View reviewed changes

dean0x merged commit 2bfa043 into main Mar 20, 2026
3 of 4 checks passed

dean0x deleted the fix/ambient-skill-loading branch March 20, 2026 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ambient): remove allowed-tools restriction so skills actually load#152

fix(ambient): remove allowed-tools restriction so skills actually load#152
dean0x merged 4 commits intomainfrom
fix/ambient-skill-loading

dean0x commented Mar 20, 2026

Uh oh!

dean0x Mar 20, 2026

Uh oh!

dean0x Mar 20, 2026

Uh oh!

dean0x Mar 20, 2026

Uh oh!

dean0x Mar 20, 2026

Uh oh!

dean0x Mar 20, 2026

Uh oh!

dean0x commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

dean0x commented Mar 20, 2026

Summary

Test plan

Uh oh!

dean0x Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

dean0x Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

dean0x Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

dean0x Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

dean0x Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

dean0x commented Mar 20, 2026

Code Review Summary

Inline Comments Created (6)

Additional Findings (60-79% confidence, see details below)

Recommendation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant